check if a source NAT IP address is needed before assigning one by DaanHoogland · Pull Request #12408 · apache/cloudstack

DaanHoogland · 2026-01-12T15:30:39Z

Description

This PR...

Fixes: #10122

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov · 2026-01-12T15:38:41Z

Codecov Report

❌ Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.24%. Comparing base (ef1aaa0) to head (519b8cf).
⚠️ Report is 9 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...n/java/com/cloud/network/IpAddressManagerImpl.java	0.00%	17 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #12408   +/-   ##
=========================================
  Coverage     16.23%   16.24%           
  Complexity    13381    13381           
=========================================
  Files          5657     5657           
  Lines        498947   499002   +55     
  Branches      60555    60571   +16     
=========================================
+ Hits          81025    81039   +14     
- Misses       408889   408924   +35     
- Partials       9033     9039    +6

Flag	Coverage Δ
uitests	`4.03% <ø> (+0.02%)`	⬆️
unittests	`17.09% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DaanHoogland · 2026-01-13T07:47:50Z

@blueorangutan package

blueorangutan · 2026-01-13T07:50:04Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-01-13T09:17:35Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16344

Copilot

Pull request overview

This PR addresses issue #10122 regarding incorrect IP address labeling as "Source NAT" in Routed VPC networks. The main change introduces logic to check if a network is routed before assigning a source NAT IP address, which prevents unnecessary source NAT IP allocation for routed networks where Source NAT service is not applicable.

Changes:

Adds new isRouted() method to determine if a network uses routing mode
Updates assignSourceNatIpAddressToGuestNetwork() to conditionally assign source NAT based on routing mode
Updates isSourceNatAvailableForNetwork() to treat networks with routing mode as having shared source NAT
Includes extensive code cleanup: removes unused imports and fields, applies diamond operators, and simplifies conditional logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

weizhouapache · 2026-01-13T13:24:05Z

@DaanHoogland
as I understand, #10122 is a UI issue.
Does this PR fix other issues ?

DaanHoogland · 2026-01-13T13:27:45Z

@DaanHoogland as I understand, #10122 is a UI issue. Does this PR fix other issues ?

it is not a pure UI issue. The IP address is marked as source NAT in the DB. I am not sure if this addresses all scenarios yet, but it will prevent marking the primary IP for a ROUTED-mode network as source NAT.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

Copilot · 2026-01-14T14:54:28Z

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

+    private boolean isRouted(Network guestNetwork) {
+        VpcOffering vpcOffer = null;
+        NetworkOffering netOffer = _networkOfferingDao.findById(guestNetwork.getNetworkOfferingId());
+        if (netOffer == null) {
+            throw new CloudRuntimeException("network without offering found???");
+        }
+        if (netOffer.isForVpc() && guestNetwork.getVpcId() != null) {
+            VpcVO vpc = _vpcDao.findById(guestNetwork.getVpcId());
+            if (vpc != null) {
+                vpcOffer = vpcOfferingDao.findById(vpc.getVpcOfferingId());
+            }
+        }
+        return netOffer.getRoutingMode() != null || (vpcOffer != null && vpcOffer.getRoutingMode() != null);
+    }


The new isRouted method lacks test coverage. Since the repository has comprehensive test coverage for IpAddressManagerImpl (see IpAddressManagerTest.java), consider adding unit tests to verify:

The method correctly identifies routed networks based on NetworkOffering routing mode

The method correctly identifies routed VPC networks based on VPC offering routing mode

Edge cases like null VPC or null VPC offering are handled properly

weizhouapache · 2026-01-14T15:22:52Z

@DaanHoogland as I understand, #10122 is a UI issue. Does this PR fix other issues ?

it is not a pure UI issue. The IP address is marked as source NAT in the DB. I am not sure if this addresses all scenarios yet, but it will prevent marking the primary IP for a ROUTED-mode network as source NAT.

ok @DaanHoogland
it is becoming a PR with big impact then.

as I understand, what @msinhore wanted is the change of label of public IP. source nat of routed network confuses users, need to change to "external gateway" or so
what ACS saves into database and how it is processed in the service layer are not very important.

DaanHoogland · 2026-01-14T15:53:00Z

@DaanHoogland as I understand, #10122 is a UI issue. Does this PR fix other issues ?

it is not a pure UI issue. The IP address is marked as source NAT in the DB. I am not sure if this addresses all scenarios yet, but it will prevent marking the primary IP for a ROUTED-mode network as source NAT.

ok @DaanHoogland it is becoming a PR with big impact then.

as I understand, what @msinhore wanted is the change of label of public IP. source nat of routed network confuses users, need to change to "external gateway" or so what ACS saves into database and how it is processed in the service layer are not very important.

isSourceNat is a flag returned to the UI, if we do not change it in the service we have to honour that flag conditionally. Not a very clean solution either and technical debt at the same time. I would suggest we fix it good or not.

weizhouapache · 2026-01-14T16:19:31Z

@DaanHoogland as I understand, #10122 is a UI issue. Does this PR fix other issues ?

it is not a pure UI issue. The IP address is marked as source NAT in the DB. I am not sure if this addresses all scenarios yet, but it will prevent marking the primary IP for a ROUTED-mode network as source NAT.

ok @DaanHoogland it is becoming a PR with big impact then.
as I understand, what @msinhore wanted is the change of label of public IP. source nat of routed network confuses users, need to change to "external gateway" or so what ACS saves into database and how it is processed in the service layer are not very important.

isSourceNat is a flag returned to the UI, if we do not change it in the service we have to honour that flag conditionally. Not a very clean solution either and technical debt at the same time. I would suggest we fix it good or not.

ok.
please bear in mind, Routed network requires a public IP for VR, as external gateway IP.
you can set isSourceNat to false in response, but may need to add another flag, for example isGateway which is set to true.

a simple workaround on UI

if IP is source nat,
- if network is Routed, display "external gateway"
- if not, display "source nat"
xxx

boring-cyborg bot added the component:networking label Jan 12, 2026

DaanHoogland requested a review from Copilot January 13, 2026 13:02

Copilot started reviewing on behalf of DaanHoogland January 13, 2026 13:02 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

DaanHoogland requested a review from Copilot January 13, 2026 13:21

Copilot started reviewing on behalf of DaanHoogland January 13, 2026 13:22 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

DaanHoogland force-pushed the ghi10122-routedModeWithSourceNAT branch from e8cebc6 to 7274c9e Compare January 14, 2026 14:44

DaanHoogland requested a review from Copilot January 14, 2026 14:44

Copilot started reviewing on behalf of DaanHoogland January 14, 2026 14:45 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

check if a source NAT IP address is needed before assigning one

519b8cf

DaanHoogland force-pushed the ghi10122-routedModeWithSourceNAT branch from 7274c9e to 519b8cf Compare January 14, 2026 15:19

Conversation

DaanHoogland commented Jan 12, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

codecov bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Jan 13, 2026

Uh oh!

blueorangutan commented Jan 13, 2026

Uh oh!

blueorangutan commented Jan 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weizhouapache commented Jan 13, 2026

Uh oh!

DaanHoogland commented Jan 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Jan 14, 2026

Uh oh!

DaanHoogland commented Jan 14, 2026

Uh oh!

weizhouapache commented Jan 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 12, 2026 •

edited

Loading